JIT: include element info when comparing array address trees - #131485
Merged
Conversation
GenTree::Compare ignored the element size on GT_ARR_ELEM and all of the element metadata on GT_ARR_ADDR and GT_INDEX_ADDR, so tail merge could fold two array element addresses that differ only in element type or scale. Also hash these fields, and move the unreachable GT_INDEX_ADDR case in gtHashValue to the binop arm where it can actually run. Fixes dotnet#131459. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Member
Author
|
@EgorBo PTAL No diffs expected. |
github-actions
Bot
added
the
area-CodeGen-coreclr
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a JIT correctness issue where GenTree::Compare (and related hashing) did not include key element metadata for array-address-related nodes, allowing tail-merge/CSE to incorrectly treat distinct array element address computations as identical. It also adds a JIT regression test covering the reported miscompile and a related GT_ARR_ADDR metadata scenario.
Changes:
- Extend
GenTree::Compareto include element metadata when comparingGT_ARR_ELEM,GT_ARR_ADDR, andGT_INDEX_ADDR. - Extend
Compiler::gtHashValueto hash the same element metadata forGT_ARR_ELEM,GT_ARR_ADDR, andGT_INDEX_ADDR(and moveGT_INDEX_ADDRhashing to the correct operator-kind arm). - Add a regression test (
Runtime_131459) and include it inRegression_ro_2.csproj.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/coreclr/jit/gentree.cpp | Include array element/type/scale metadata in GenTree::Compare and gtHashValue for relevant tree kinds to prevent incorrect folding/merging. |
| src/tests/JIT/Regression/JitBlue/Runtime_131459/Runtime_131459.cs | Add regression coverage for GT_ARR_ELEM element-size compare and GT_ARR_ADDR element-type/class-handle metadata compare. |
| src/tests/JIT/Regression/Regression_ro_2.csproj | Add the new regression test file to the compilation list. |
GenTree::Compare looked at the element metadata on GT_INDEX_ADDR but not at GTF_INX_RNGCHK / GTF_INX_ADDR_NONNULL, so tail merge could fold a GetArrayDataReference expansion with an ordinary ldelema and drop the range check. Compare and hash those bits too. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/coreclr/jit/gentree.cpp:3624
- Similarly, in the binary ExOp arm of
gtHashValue, the comment "For the ones below no extra argument matters for comparison." is no longer accurate now thatGT_INDEX_ADDRhashes multiple metadata fields. Please update the comment to reflect the new structure.
src/tests/JIT/Regression/JitBlue/Runtime_131459/Runtime_131459.cs:56 - The
AddressBigcoverage looks ineffective for this fix:S256/S512have element sizes 256/512, but the JIT importer rejects expanding mdarray intrinsics whenarrayElemSizedoesn’t fit inunsigned char(seesrc/coreclr/jit/importercalls.cpparound theFitsIn<unsigned char>(arrayElemSize)check). That means these cases likely won’t produceGT_ARR_ELEM, so they won’t exercise the newGenTree::Compare/gtHashValueelement-size handling. Consider removing this section or changing the types to sizes <= 255 so it stays on the intrinsic/GT_ARR_ELEMpath.
[MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
private static ref byte AddressBig(bool small, object array)
{
if (small)
{
src/coreclr/jit/gentree.cpp:3569
- In
gtHashValue, the comment "For the ones below no extra argument matters for comparison." is now misleading becauseGT_ARR_ADDRimmediately below does contribute additional metadata to the hash. Updating the comment avoids future confusion when adding more ExOp cases.
This issue also appears on line 3621 of the same file.
EgorBo
reviewed
Jul 29, 2026
EgorBo
approved these changes
Jul 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GenTree::Compare ignored the element size on GT_ARR_ELEM and all of the element metadata on GT_ARR_ADDR and GT_INDEX_ADDR, so tail merge could fold two array element addresses that differ only in element type or scale. Also hash these fields, and move the unreachable GT_INDEX_ADDR case in gtHashValue to the binop arm where it can actually run.
Fixes #131459.